Add initial Blackboard support #7
Conversation
dkroenke
left a comment
dkroenke
left a comment
There was a problem hiding this comment.
An entry in the Changelog would be good.
To keep the git history a bit more clean you could update the feature branches to latest main via rebase instead of merge. That avoids the merge commits in the history. In iceoryx2 we do it the same way.
There was a problem hiding this comment.
Could you add a small Readme on how to run the example and what is happening there?
A link to https://ekxide.github.io/iceoryx2-book/main/fundamentals/messaging-patterns/blackboard.html would be help here.
There was a problem hiding this comment.
ok...will do in the future
There was a problem hiding this comment.
@dkroenke please wait with the review while it is still in DRAFT state
|
Adding @FerdinandSpitzschnueffler for the review of the Backboard components |
FerdinandSpitzschnueffler
left a comment
FerdinandSpitzschnueffler
left a comment
There was a problem hiding this comment.
I'm not familiar with C#, so I mainly focused on the blackboard API and not on the implementation. The PR looks good to me, but some function names have changed with the last release.
| /// Gets the current value from the blackboard entry. | ||
| /// </summary> | ||
| /// <returns>A Result containing the current value or an error.</returns> | ||
| public unsafe Result<TValue, Iox2Error> Get() |
There was a problem hiding this comment.
Do we need to return a Result here? Get shouldn't fail and it seems that we never return an Err in the function.
| /// </summary> | ||
| /// <param name="value">The new value to set.</param> | ||
| /// <returns>A Result indicating success or an error.</returns> | ||
| public unsafe Result<Unit, Iox2Error> Update(TValue value) |
There was a problem hiding this comment.
With the last release, update() was renamed to update_with_copy(). Could you please rename this method as well for consistency?
| /// </summary> | ||
| /// <param name="value">The new value to set.</param> | ||
| /// <returns>A Result indicating success or an error.</returns> | ||
| public unsafe Result<Unit, Iox2Error> Update(TValue value) |
There was a problem hiding this comment.
Do we need to return a Result?
| /// </summary> | ||
| /// <typeparam name="TKey">The type of the key.</typeparam> | ||
| /// <typeparam name="TValue">The type of the value.</typeparam> | ||
| public sealed class EntryValue<TKey, TValue> : IDisposable |
There was a problem hiding this comment.
Could you please rename the class to EntryValueUninit for consistency?
| /// Gets a mutable reference to the payload for in-place modification. | ||
| /// </summary> | ||
| /// <returns>A reference to the payload.</returns> | ||
| public unsafe ref TValue PayloadMut() |
There was a problem hiding this comment.
Could you please rename the function to ValueMut for consistency?
| /// Writes a value to the loaned memory. | ||
| /// </summary> | ||
| /// <param name="value">The value to write.</param> | ||
| public unsafe void Write(TValue value) |
There was a problem hiding this comment.
With the last release, write and update have been combined to update_with_copy. Could you please adapt this here as well? FYI: https://github.com/eclipse-iceoryx/iceoryx2/blob/main/doc/release-notes/iceoryx2-v0.8.0.md#api-breaking-changes, item 9.
| var entryResult = reader.Entry<TValue>(key); | ||
|
|
||
| if (entryResult.IsOk) |
There was a problem hiding this comment.
Currently, blackboard entries cannot be removed, so it should be fine to move these lines outside the while loop.
| var entryResult = reader.Entry<TValue>(key); | ||
|
|
||
| if (entryResult.IsOk) | ||
| { | ||
| using var entry = entryResult.Unwrap(); |
- Introduced `BlackboardHelpers` for alignment handling. - Refactored key alignment logic across Blackboard classes. - Added thread safety remarks to `Writer`, `Reader`, and `BlackboardServiceBuilder`. - Enhanced `BlackboardServiceBuilder` with input validation. - Optimized memory usage in key-value operations via stack allocation. - Updated README with examples and documentation for Blackboard API. - Incorporated detailed comments and ensured proper resource disposal.
3 participants
No description provided.